Implement Tor client updater as component extension #316

emerick · 2018-08-09T19:05:46Z

Fixes brave/brave-browser#724

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.

Test Plan:

Extension download and installation
- Start with a clean profile
- Launch Brave
- Visit chrome://components
- Verify that Tor Client Updater is listed and version number is not 0.0.0.0
- Verify that name of component contains platform identifier (Windows/Mac/Linux)
Command-line switch to disable extension
- Start with a clean profile
- Launch Brave with --disable-tor-client-updater-extension
- Visit chrome://components and verify that Tor Client Updater is not listed

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions

emerick · 2018-08-09T19:06:06Z

cc: @riastradh-brave

bridiver · 2018-08-23T01:52:11Z

browser/extensions/brave_tor_client_updater.cc

+
+void BraveTorClientUpdater::InitExecutablePath(
+    const base::FilePath& install_dir) {
+  base::FileEnumerator traversal(install_dir, false,


This should be flagged for security audit. We shouldn't be using a wildcard here to find files, we need to look for a specific file and match the contents against a checksum cc @diracdeltas

Ah, good point. I implemented it like that because the Tor client embeds versioning and target platform information in its executable name (example: tor-0.3.3.8-win32-brave-5) and I didn't want to tightly couple the updater with a specific Tor client. I think one thing we can do here is to change the name of the Tor client to something a bit more generic when packaging it in the extension (e.g., brave-tor-client or something like that).

A checksum makes sense, but I think it would mean that we couldn't update the Tor client without also updating Brave. I think that's at least part of the problem the extension was meant to address.

cc: @riastradh-brave and @bbondy for any other thoughts here.

we do validate the crx so the checksum can be included along with the exe

Thank you for flagging, I am assigning to @riastradh-brave for retroactive security review.

I would be perfectly fine with hard-coding a hash -- that's what we do currently. As I understand it, the only real specific motivation we have for doing a separate extension is to avoid antivirus software that is unhappy if Brave is bundled with a tor executable -- but even then, I'm not sure have any reports of antivirus software complaining that it's there; all the reports I'm aware of are complaints that it got executed.

Generally, we put out updates more frequently than tor does anyway.

I see what's going on here. @emerick Can we just use a fixed pathname at a fixed location in the extension's install directory for the executable file? The only reason the file that is published via S3 has a different name is that we are publishing several different files, one for each platform.

@riastradh-brave Yeah, I'll update the packager to use a more generic name for the executable and then we'll just look specifically for that.

bridiver · 2018-08-23T01:55:10Z

browser/extensions/brave_extension_management.cc

+  const base::CommandLine& command_line =
+      *base::CommandLine::ForCurrentProcess();
+  if (!command_line.HasSwitch(switches::kDisableTorClientUpdaterExtension))
+    g_brave_browser_process->tor_client_updater()->Register();


do we need this in BraveBrowserProcess? Seems like it could just live here?

My thinking here was that BraveBrowserProcess would be the owner of long-lived services like the Tor client updater and then we'd just access it here via its getter so that we can register the extension on startup.

Are you suggesting to move the Register function into BraveExtensionManagement (or even the entire updater)? I haven't followed the code paths all the way through, but it seems like that could work. My only concerns would be that the registration happens at the correct time at startup (I definitely had a few issues with this before I settled on having BraveBrowserProcess own them) and that the updaters are easily accessible to other parts of the code.

the tor client updater isn't a long-lived service, the component updater is the long-lived service and you are just registering things to be updated by it

I think maybe there is some confusion here because of the naming. I would use the WidevineCDMComponentInstaller as an example for this

OK, I see what you're saying about component updater. I did model this on some of the existing components (not specifically WidevineCDMComponentInstaller, but something similar). In our code, BraveComponentInstaller is fulfilling the same role as WidevineCDMComponentInstaller (i.e., it implements the installer policy and provides a register function).

It seems like making BraveComponentInstaller generic was a mistake; instead, it should be specific to this component and contain all of the logic that gets executed upon registration. Also, it seems like registration should occur in chrome_browser_main or somewhere similar. Is that the approach you're thinking of?

probably in BraveBrowserMainExtraParts::PreMainMessageLoopRun

btw we'll want this opt in and not always registered. Installed first time switch is turned on in a private tab.
@cezaraugusto can you track this work and do the fix for the private tab UI? You could land a dead switch control any time.

riastradh-brave · 2018-08-24T22:19:29Z

Is the security model of extensions documented anywhere? I gather that there is a public key for verifying signatures, but I'm interested in making sure that there's, e.g., no vector for rollback attacks with valid signatures.

riastradh-brave · 2018-08-24T22:24:57Z

With apologies for asking a basic question after the work has already been done and merged... What is the advantage of using a component extension instead of simply downloading a file at a standard URL with a hard-coded hash, and updating the browser if we ever need to update tor? Are we worried about making sure users of old versions of Brave can update the tor daemon?

bridiver · 2018-08-27T18:11:27Z

@riastradh-brave the advantage is that we don't have to duplicate a bunch of code for checking updates, downloading, verifying and installing those updates. Is there some reason why we wouldn't want to use it?

bridiver · 2018-08-28T16:44:30Z

@riastradh-brave I believe crx3 addresses some related packaging issues and is now required for chromium 70

@ryanml

Changes: https://github.com/brave/brave-ui/compare/5c2d8a0f669af2e9a68ce0fe38d49a116bc52315..861153d09e025ca98d6fae11bb51aa458c4d8519 ``` * 861153d - (21 hours ago) Merge pull request #247 from brave/c71-clock-period — Pete Miller (HEAD -> brave-core-0.58.x, origin/brave-core-0.58.x) * 4195647 - (2 hours ago) Merge pull request #316 from brave/sync-revert-58 — Pete Miller |\ | * cad6678 - (2 hours ago) Revert "sync v2" — Cezar Augusto | * d134401 - (2 hours ago) Revert "Merge pull request #308 from brave/sync_img" — Cezar Augusto | * 52aef38 - (2 hours ago) Revert "allow string type for some siteBanner props" — Cezar Augusto |/ * 5f4a8b1 - (3 hours ago) allow string type for some siteBanner props auditor: @ryanml - currentAmout, onAmountSelection and onDonate accept strings as well this is a temp fix only included in 0.58.x — Cezar Augusto * f0569c8 - (4 days ago) do not count first letter if space in textareaclipboard — Cezar Augusto * 69ef8be - (5 days ago) Merge pull request #308 from brave/sync_img — Cezar Augusto * 854d181 - (3 weeks ago) sync v2 — Cezar Augusto * b35662c - (4 days ago) update snapshot for walletSummary — Cezar Augusto * a85206d - (5 days ago) Merge pull request #309 from brave/welcome_img — Pete Miller ```

emerick added 4 commits August 8, 2018 10:16

Factor BraveComponentExtension base class out of BaseBraveShieldsService

8821f4c

Create extension to manage updating the Tor client

ed7c0ab

Add command-line switch to disable Tor client updater extension

971ecf8

Add browser test for Tor client updater extension

82e342d

emerick self-assigned this Aug 9, 2018

emerick requested a review from bbondy August 9, 2018 19:05

emerick changed the title ~~Tor extension~~ Implement Tor client updater as component extension Aug 9, 2018

bbondy approved these changes Aug 9, 2018

View reviewed changes

bbondy merged commit 7833d5c into master Aug 9, 2018

bsclifton deleted the tor-extension branch August 9, 2018 20:08

bridiver reviewed Aug 23, 2018

View reviewed changes

diracdeltas requested a review from riastradh-brave August 23, 2018 18:39

darkdh mentioned this pull request Aug 25, 2018

IPFS Integration - Roadmap and discussion brave/brave-browser#819

Closed

drbh mentioned this pull request Sep 8, 2018

WIP: IPFS extension #425

Closed

11 tasks

darkdh mentioned this pull request Sep 11, 2018

Tor profile service #357

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Tor client updater as component extension #316

Implement Tor client updater as component extension #316

emerick commented Aug 9, 2018 •

edited

Loading

emerick commented Aug 9, 2018

bridiver Aug 23, 2018

emerick Aug 23, 2018

bridiver Aug 23, 2018

diracdeltas Aug 23, 2018

riastradh-brave Aug 24, 2018

riastradh-brave Aug 24, 2018

riastradh-brave Aug 24, 2018 •

edited

Loading

emerick Aug 24, 2018

bridiver Aug 23, 2018

emerick Aug 23, 2018

bridiver Aug 23, 2018

bridiver Aug 23, 2018

emerick Aug 23, 2018

bridiver Aug 23, 2018

bbondy Aug 24, 2018

riastradh-brave commented Aug 24, 2018

riastradh-brave commented Aug 24, 2018

bridiver commented Aug 27, 2018 •

edited

Loading

bridiver commented Aug 28, 2018

Implement Tor client updater as component extension #316

Implement Tor client updater as component extension #316

Conversation

emerick commented Aug 9, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

emerick commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riastradh-brave Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riastradh-brave commented Aug 24, 2018

riastradh-brave commented Aug 24, 2018

bridiver commented Aug 27, 2018 • edited Loading

bridiver commented Aug 28, 2018

emerick commented Aug 9, 2018 •

edited

Loading

riastradh-brave Aug 24, 2018 •

edited

Loading

bridiver commented Aug 27, 2018 •

edited

Loading